Add completion for throw keyword by nojaf · Pull Request #7905 · rescript-lang/rescript

nojaf · 2025-09-19T08:28:39Z

pkg-pr-new · 2025-09-19T08:37:36Z

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7905

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7905

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7905

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7905

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7905

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7905

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7905

commit: 6406d88

nojaf · 2025-09-19T09:01:37Z

analysis/src/CompletionBackEnd.ml

      | None -> []))

+(* Collect exception constructor names from cmt infos. *)
+let exceptions_from_cmt_infos (infos : Cmt_format.cmt_infos) :


This part is vibe coded I'm afraid.
Please review it and let me know if there is a better way.

Can a typed tree AST iterator be used here instead of the hand rolled traversal functions below? Would feel more idiomatic, and make the code clearer as you could just add a visitor for the relevant extension nodes.

nojaf · 2025-09-19T09:01:55Z

analysis/src/CompletionBackEnd.ml

+(* Predefined Stdlib/Pervasives exceptions. *)
+let predefined_exceptions : (string * bool) list =
+  [
+    ("Not_found", true);


As found on https://rescript-lang.org/docs/manual/v12.0.0/exception#built-in-exceptions

I can't help but wonder whether we should be encouraging using these builtins. They're really just heritage from the OCaml era. So I think a case could be made to not include them at all.

Copilot

Pull Request Overview

This PR adds completion support for the throw keyword in ReScript, providing suggestions for both built-in and user-defined exceptions when typing "throw".

Implements completion functionality that suggests exception constructors when typing "throw"
Adds logic to extract exception information from compiled module files (CMT)
Includes predefined standard library exceptions with appropriate argument placeholders

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
analysis/src/CompletionBackEnd.ml	Core implementation of throw completion logic including exception extraction and completion generation
tests/analysis_tests/tests/src/Throw.res	Test file with custom exceptions for validation
tests/analysis_tests/tests/src/expected/Throw.res.txt	Expected completion output for throw keyword test
tests/analysis_tests/tests/src/expected/CompletionJsxProps.res.txt	Updated test expectations including new Throw module
tests/analysis_tests/tests/src/expected/Completion.res.txt	Updated test expectations including new Throw module
CHANGELOG.md	Documentation of the new feature

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

analysis/src/CompletionBackEnd.ml

zth

Nice! Some initial questions.

zth · 2025-09-23T18:59:16Z

analysis/src/CompletionBackEnd.ml

+(* Predefined Stdlib/Pervasives exceptions. *)
+let predefined_exceptions : (string * bool) list =
+  [
+    ("Not_found", true);


I can't help but wonder whether we should be encouraging using these builtins. They're really just heritage from the OCaml era. So I think a case could be made to not include them at all.

analysis/src/CompletionBackEnd.ml

zth · 2025-09-23T19:02:42Z

analysis/src/CompletionBackEnd.ml

+    match (result, path) with
+    | [], [prefix] when Utils.startsWith "throw" prefix ->
+      completionsForThrow ~env ~full
+    | _ -> result)


This should probably be done in a "safer" way that matches by type and not by name. We'd need to check if this thing actually resolves to Pervasives.throw.

Also, why Utils.startsWith? Is it not supposed to be an exact match?

@nojaf this has not been answered I believe.

We'd need to check if this thing actually resolves to Pervasives.throw.

If you only have thr you can't do that.

Also, why Utils.startsWith? Is it not supposed to be an exact match?

As mentioned, I want the completions to show up while I'm typing towards the word throw. Only having it once the full word is typed I find unintuitive.

zth · 2025-09-23T19:05:42Z

analysis/src/CompletionBackEnd.ml

      | None -> []))

+(* Collect exception constructor names from cmt infos. *)
+let exceptions_from_cmt_infos (infos : Cmt_format.cmt_infos) :


Can a typed tree AST iterator be used here instead of the hand rolled traversal functions below? Would feel more idiomatic, and make the code clearer as you could just add a visitor for the relevant extension nodes.

nojaf · 2025-09-24T13:34:04Z

Maybe this should more autocomplete JsError.throwWithMessage("Hello!") and JsExn.throw("some non-error value!") instead. (docs)

I cannot ever remember those things and I basically want to have something show up when I type the throw keyword. (Or I'm typing toward that keyword, should trigger on thr as well).

I'm okay with leaving the camels behind.

mediremi

I agree with showing completions for JsError.throwWithMessage and JsExn.throw in addition to throw 👍

zth

Still some outstanding questions and comments from me. Also, is this just for the throw case, without parenthesis and args? A natural extension of this would be to also complete inside of throw(<com>). Unless that already works ofc.

zth · 2025-09-26T10:30:18Z

analysis/src/CompletionBackEnd.ml

+    match (result, path) with
+    | [], [prefix] when Utils.startsWith "throw" prefix ->
+      completionsForThrow ~env ~full
+    | _ -> result)


@nojaf this has not been answered I believe.

zth · 2025-09-26T10:31:43Z

analysis/src/ProcessCmt.ml

+        (match item.str_desc with
+        | Tstr_exception _ -> in_toplevel_exception := true
+        | _ -> ());
+        ()


Why is there an extra unit here...?

zth · 2025-09-26T10:31:50Z

analysis/src/ProcessCmt.ml

+        (match item.sig_desc with
+        | Tsig_exception _ -> in_toplevel_exception := true
+        | _ -> ());
+        ()


Same, extra unit.

zth · 2025-09-26T10:32:41Z

analysis/src/ProcessCmt.ml

+    let module Iter = TypedtreeIter.MakeIterator (struct
+      include TypedtreeIter.DefaultIteratorArgument


Do we use the functor based approach elsewhere in the code base? Or is there no way for the typed tree to just define a mapper like we do for the AST mapper? I don't remember how this works exactly.

Yes, this was weird to me as well, it is a departure from what we have on the untyped side, but this seems to be the mapper we have?

zth · 2025-09-26T10:35:41Z

analysis/src/ProcessCmt.ml

+    (* Only collect top-level exception declarations (Tstr_exception/Tsig_exception).
+     Avoid picking up exceptions from Texp_letexception by tracking context. *)


In theory I guess the most robust approach would be to collect and use whatever defined exceptions are in scope at the current position. Collecting at the top level is good and simple, but you could also define them in sub modules, and so on.

It might be worth exploring making exceptions part of the tracked scope instead, and track them just like we track types and values today when doing completion. Then we could just extend the current mechanism a bit, and hopefully get the correct scope tracking for free.

nojaf · 2026-01-20T07:57:01Z

Hi @zth, could you have another pass on this?

nojaf commented Sep 19, 2025

View reviewed changes

nojaf marked this pull request as ready for review September 19, 2025 09:04

nojaf requested review from Copilot, cristianoc and zth September 19, 2025 09:05

Copilot AI reviewed Sep 19, 2025

View reviewed changes

analysis/src/CompletionBackEnd.ml Outdated Show resolved Hide resolved

analysis/src/CompletionBackEnd.ml Outdated Show resolved Hide resolved

zth requested changes Sep 23, 2025

View reviewed changes

nojaf added the editor Everything concerning the analysis binary label Sep 26, 2025

mediremi approved these changes Sep 26, 2025

View reviewed changes

nojaf requested a review from zth September 26, 2025 09:57

zth requested changes Sep 26, 2025

View reviewed changes

nojaf added 9 commits January 20, 2026 08:42

Add completion for throw keyword

f387383

Try completions if path didn't have any results

f18e8f2

Add changelog entry

532a773

extract to helper function

816e305

Use typed tree ast iterator

a352e21

Extract cmt helper to ProcessCmt

3ff9f93

Remove ReScript exceptinons and add hardcoded throw functions.

f3f5e09

Update snapshot

d146f7f

Apply code review feedback

d0d8688

nojaf force-pushed the complete-throw branch from 7dc6ff1 to d0d8688 Compare January 20, 2026 07:55

nojaf requested a review from zth January 20, 2026 07:56

nojaf added this to the v13 milestone Jan 20, 2026

Merge branch 'master' into complete-throw

6406d88

		let module Iter = TypedtreeIter.MakeIterator (struct
		include TypedtreeIter.DefaultIteratorArgument

		(* Only collect top-level exception declarations (Tstr_exception/Tsig_exception).
		Avoid picking up exceptions from Texp_letexception by tracking context. *)

Conversation

nojaf commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

zth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Sep 24, 2025

Uh oh!

mediremi left a comment

Choose a reason for hiding this comment

Uh oh!

zth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojaf commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nojaf commented Sep 19, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 19, 2025 •

edited

Loading